Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use the infobar for WDP CTA instead of dialog #15822

Merged
merged 3 commits into from
Nov 23, 2022
Merged

Conversation

simonhong
Copy link
Member

@simonhong simonhong commented Nov 7, 2022

fix brave/brave-browser#26576

Deleted previous WDP dialog.
Use Infobar UI for WDP CTA instead of modal dialog.
And removed bottom shadow from infobar container.

Light mode
Screen Shot 2022-11-22 at 2 29 34 PM
Dark mode
Screen Shot 2022-11-22 at 2 30 00 PM

This custom infobar has below three state based on width.

Wide layout with wide horizontal padding
Screen Shot 2022-11-22 at 2 31 36 PM

Wide layout with narrow horizontal padding
Screen Shot 2022-11-22 at 2 32 00 PM

Narrow layout
Screen Shot 2022-11-22 at 2 32 10 PM

Screen_Recording_2022-11-22_at_2_33_37_PM_AdobeExpress

Resolves

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

npm run test brave_unit_tests -- --filter=WebDiscoveryCTATest*
npm run test brave_browser_tests -- --filter=WebDiscoveryTest*

  1. Launch brave with any profile(clean or existing)
  2. Set non Brave Search as a default search engine
  3. Load https://search.brave.com/ or with any query like https://search.brave.com/search?q=Brave&source=web
  4. Check WDP infobar is not shown
  5. Set Brave Search as a default search engine and enable WDP via brave://settings/?search=web+discovery
  6. Repeat step 3 and check WDP infobar is not shown
  7. disable WDP via brave://settings/?search=web+discovery
  8. Repeat step3 and check WDP infobar is shown
  9. Click x and check infobar is hidden
  10. InfoBar is shown 5 times at most and once per day
  11. To make infobar show again, edit last_displayed time like deleting last digit to simulate time passed more than 1 day from user dir's Preferences file after shutdown
      "web_discovery": {
         "cta_state": {
            "count": 2,
            "dismissed": true,
            "id": "v1",
            "last_displayed": "13313568560800197"
         }
      },
  1. Relaunch Brave and check infobar is shown again when search.brave.com is loaded
  2. Click No thanks and check infobar is not shown anymore
  3. When new cta id is set, infobar will be shown although it's dimissed already
  4. To simulate new cta id, change id in above Preferences to any other string
  5. Check infobar is shown again when loading search.brave.com

@simonhong simonhong self-assigned this Nov 7, 2022
@github-actions github-actions bot added the potential-layer-violation-fixes This PR touches a BUILD.gn file with check_includes=false label Nov 7, 2022
@simonhong simonhong force-pushed the use_infobar_for_wdp_cta branch 14 times, most recently from 34ce2d8 to 1493918 Compare November 17, 2022 02:54
We'll use infobar for WDP cta.
@simonhong simonhong force-pushed the use_infobar_for_wdp_cta branch 2 times, most recently from 916bdad to 31fd043 Compare November 22, 2022 01:25
@simonhong simonhong force-pushed the use_infobar_for_wdp_cta branch 2 times, most recently from d977aa2 to d5d8c77 Compare November 22, 2022 03:01
@simonhong simonhong marked this pull request as ready for review November 22, 2022 05:59
@simonhong simonhong requested a review from a team as a code owner November 22, 2022 05:59
@simonhong simonhong force-pushed the use_infobar_for_wdp_cta branch from d5d8c77 to 714b52b Compare November 22, 2022 06:00
Copy link
Member

@goodov goodov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

chromium_src lgtm

@github-actions
Copy link
Contributor

⚠️ PR head is an unsigned commit
commit: 714b52b
reason: unsigned
Please follow the handbook to configure commit signing
cc: @simonhong

Copy link
Collaborator

@rebron rebron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++ Looks good @simonhong

Copy link
Contributor

@sangwoo108 sangwoo108 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@simonhong
Copy link
Member Author

Thanks all for review :)

@simonhong simonhong merged commit e899f67 into master Nov 23, 2022
@simonhong simonhong deleted the use_infobar_for_wdp_cta branch November 23, 2022 00:14
@github-actions github-actions bot added this to the 1.47.x - Nightly milestone Nov 23, 2022
@@ -10,6 +12,12 @@ source_set("brave_wayback_machine") {
"brave_wayback_machine_infobar_throbber.h",
"brave_wayback_machine_infobar_view.cc",
"brave_wayback_machine_infobar_view.h",
"custom_styled_label.cc",
"custom_styled_label.h",
"web_discovery_infobar_content_view.cc",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this in brave_wayback_machine?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
potential-layer-violation-fixes This PR touches a BUILD.gn file with check_includes=false
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update Web Discovery Project promo to use infobar instead of dialog
5 participants